src/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.java (1)

27-35: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add ownership validation integration tests for MCP memory tools.

The test setup now includes OwnershipValidator, but no tests verify the ownership validation integration. For a security feature preventing IDOR attacks, explicit test coverage is essential.

Consider adding tests for:

• validateUserAccess is called with correct identity and userId parameters
• Non-owner receives ForbiddenException when attempting to access another user's memories
• Admin bypass behavior (if applicable to MCP tools)
• Owner can successfully access their own memories after ownership check

Similar ownership tests were added to RestGroupConversationTest (lines 226-299 in that file) and can serve as a pattern.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.java` around lines
27 - 35, Add unit tests that exercise McpMemoryTools' ownership integration: in
the test class (setUp uses mocked OwnershipValidator and SecurityIdentity) add
tests that 1) verify OwnershipValidator.validateUserAccess is invoked with the
SecurityIdentity mock and the target userId when calling the MCP methods that
load user memories, using Mockito.verify; 2) stub validateUserAccess to throw a
ForbiddenException and assert the MCP method propagates/throws
ForbiddenException for non-owners; 3) stub validateUserAccess to allow access
and assert owners can read their memories; and 4) if admin bypass behavior
exists, stub SecurityIdentity as admin and assert validateUserAccess is not
blocking (or verify bypass path). Target methods/classes: McpMemoryTools and
OwnershipValidator.validateUserAccess; use the existing userMemoryStore and
jsonSerialization mocks to drive responses and Mockito.verify/when to assert
behavior.

src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java (1)

139-146: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Arm the AsyncResponse timeout before the ownership lookup.

Lines 143 and 163 call validateConversationOwnership(conversationId) before Lines 171-172 register the timeout handler. Since that helper does a synchronous conversationDescriptorStore.readDescriptor(...), a slow or hung descriptor lookup can keep say/rerun outside the agentTimeout window and block the request thread before the async timeout is even active.

♻️ Suggested change

+    private void configureAsyncTimeout(AsyncResponse response) {
+        response.setTimeout(agentTimeout, TimeUnit.SECONDS);
+        response.setTimeoutHandler(asyncResp -> asyncResp.resume(Response.status(Response.Status.REQUEST_TIMEOUT).build()));
+    }
+
     `@Override`
     public void rerunLastConversationStep(String conversationId, String language, Boolean returnDetailed, Boolean returnCurrentStepOnly,
                                           List<String> returningFields, final AsyncResponse response) {
         checkNotNull(conversationId, "conversationId");
         checkNotEmpty(language, "language");
+        configureAsyncTimeout(response);
         validateConversationOwnership(conversationId);
 
         sayInternal(conversationId, returnDetailed, returnCurrentStepOnly, returningFields,
                 new InputData("", Map.of(KEY_LANG, new Context(string, language))), true, response);
     }
@@
     public void sayWithinContext(final String conversationId, final Boolean returnDetailed, final Boolean returnCurrentStepOnly,
                                  final List<String> returningFields, final InputData inputData, final AsyncResponse response) {
 
         checkNotNull(conversationId, "conversationId");
         checkNotNull(inputData, "inputData");
         checkNotNull(inputData.getInput(), "inputData.input");
+        configureAsyncTimeout(response);
         validateConversationOwnership(conversationId);
 
         sayInternal(conversationId, returnDetailed, returnCurrentStepOnly, returningFields, inputData, false, response);
     }
@@
     private void sayInternal(String conversationId, Boolean returnDetailed, Boolean returnCurrentStepOnly, List<String> returningFields,
                              InputData inputData, boolean rerunOnly, AsyncResponse response) {
-
-        response.setTimeout(agentTimeout, TimeUnit.SECONDS);
-        response.setTimeoutHandler(asyncResp -> asyncResp.resume(Response.status(Response.Status.REQUEST_TIMEOUT).build()));
-
         try {
             conversationService.say(conversationId, returnDetailed, returnCurrentStepOnly, returningFields, inputData, rerunOnly, response::resume);

As per coding guidelines, "Code must be thread-safe and non-blocking. REST endpoints use JAX-RS AsyncResponse. Tasks execute synchronously but must not block for extended periods."

Also applies to: 160-173

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java` around lines
139 - 146, The AsyncResponse timeout must be armed before any synchronous
blocking calls like validateConversationOwnership(conversationId); modify
rerunLastConversationStep (and the other affected endpoint method) so that you
register the AsyncResponse timeout/timeout handler (the code that sets the agent
timeout on the provided AsyncResponse) immediately upon entry, then perform
validateConversationOwnership(conversationId) and the subsequent
sayInternal(...) call; ensure sayInternal(...) and
validateConversationOwnership(...) remain unchanged but are executed after the
timeout is registered so a hung conversationDescriptorStore.readDescriptor(...)
cannot prevent the JAX-RS timeout from firing.

Source: Coding guidelines